[vslib]Add MACsec Filters #713

Pterosaur · 2020-11-18T15:40:46Z

The document about MACsec Filters can be found at: https://github.com/Azure/SONiC/blob/88c7ada900051f73a29aaf4a28bcaf5062ae7305/doc/macsec/MACsec_hld.md#345-virtual-macsec-sai

All filters will be installed in HostInterfaceInfo to filter the EAPOL and data traffic.
The MACsecEgressFilter will forward EAPOL packets to eth device and forward data packets to MACsec device.
The MACsecIngressFilter will forward EAPOL packets to Ethernet device and drop all data packets. Because the data packets will be forwarded to MACsec device by Linux kernel.

The TrafficFilterPipes provides an interfaces to add more filters for future functions.

Signed-off-by: Ze Gan [email protected]

Signed-off-by: Ze Gan <[email protected]>

kcudnik · 2020-11-18T23:48:33Z

vslib/inc/MACsecFilter.h

+
+namespace saivs
+{
+    class MACsecFilter : public TrafficFilter


base class to new line

Done, please review it.

kcudnik · 2020-11-18T23:48:48Z

vslib/inc/MACsecFilter.h

+
+        virtual FilterStatus execute(
+            _Inout_ void *buffer,
+            _Inout_ ssize_t &length) override;


why signed size ? and not size_t ?

This function will be call after recvmsg, So I directly use the type in this code
https://github.com/Azure/sonic-sairedis/blob/425ffbcbf57fdb66160c1ba84bbec2c0f1f5e983/vslib/src/HostInterfaceInfo.cpp#L145
Should I cast it to unsigned size?

ok, this is fine, i though this size is buffer length, since then it make no sense for it to allow negative numbers
so i looked at your code, so you actually passing &length, but forward function is without (&), and you are not modifying length parameter in any way, so it should also be without (&), then recvmsg returns ssize, and its negative when error, but you done return that ether, you have your own FilterStatus error code, so i think this ssize_t& should be change to size_t in all places and if needed cast to int in recvmgs method since thats the definition

I agree your point. size_t should make more sense.
And the function execute, I think, is for processing the data, maybe it will modify the data. So the paramete of execute are with (&).
But the function forward, I think, is for forwarding data. The forwarding action will never change the data. So the parameters are without (&).

kcudnik · 2020-11-18T23:49:03Z

vslib/inc/MACsecFilter.h

+            _Inout_ void *buffer,
+            _Inout_ ssize_t &length) override;
+
+        void enable_macsec_device(bool enable);


each param needs prefix In and 1 per line

Done, please review it.

kcudnik · 2020-11-18T23:49:12Z

vslib/inc/MACsecFilter.h

+
+        void enable_macsec_device(bool enable);
+
+        void set_macsec_fd(int macsecfd);


same here 1 param per line

Done, please review it.

kcudnik · 2020-11-18T23:49:19Z

vslib/inc/MACsecFilter.h

+    protected:
+        virtual FilterStatus forward(
+            _In_ const void *buffer,
+            _In_ ssize_t length) = 0;


why ssize_t ?

Same reason as previous comment.

kcudnik · 2020-11-18T23:49:35Z

vslib/inc/MACsecIngressFilter.h

+
+namespace saivs
+{
+    class MACsecIngressFilter : public MACsecFilter


base class to new line

Done, please review it.

kcudnik · 2020-11-18T23:49:41Z

vslib/inc/MACsecIngressFilter.h

+    class MACsecIngressFilter : public MACsecFilter
+    {
+    public:
+        MACsecIngressFilter(_In_ const std::string &macsec_interface_name);


1 param per line

Done, please review it.

kcudnik · 2020-11-18T23:50:28Z

vslib/src/MACsecEgressFilter.cpp

+
+using namespace saivs;
+
+MACsecEgressFilter::MACsecEgressFilter(_In_ const std::string &macsecInterfaceName):


1 param per line

Done, please review it.

kcudnik · 2020-11-18T23:50:59Z

vslib/src/MACsecFilter.cpp

+}
+
+void MACsecFilter::enable_macsec_device(bool enable)
+{


missing SWSS_LOG_ENTER()

Done, please review it.

kcudnik · 2020-11-18T23:51:07Z

vslib/src/MACsecFilter.cpp

+}
+
+void MACsecFilter::set_macsec_fd(int macsecfd)
+{


missing swss_log_enter

Done, please review it.

kcudnik · 2020-11-18T23:51:50Z

vslib/src/TrafficFilterPipes.cpp

+            ret = filter->execute(
+                buffer,
+                length);


join 3 lines to one line ? this is small enough

Done, please review it.

kcudnik · 2020-11-18T23:51:58Z

vslib/src/TrafficFilterPipes.cpp

+        {
+            itr = m_filters.erase(itr);
+        }
+


unnecessary empty line

Done, please review it.

Signed-off-by: Ze Gan <[email protected]>

All filters will be installed in HostInterfaceInfo to filter the EAPOL and data traffic. The MACsecEgressFilter will forward EAPOL packets to eth device and forward data packets to MACsec device. The MACsecIngressFilter will forward EAPOL packets to Ethernet device and drop all data packets. Because the data packets will be forwarded to MACsec device by Linux kernel. The TrafficFilterPipes provides an interfaces to add more filters for future functions.

Add MACsec Filters

ba7ae2c

Signed-off-by: Ze Gan <[email protected]>

Pterosaur mentioned this pull request Nov 18, 2020

Macsec High level design sonic-net/SONiC#652

Merged

Pterosaur changed the title ~~[MACsec vSAI]Add MACsec Filters~~ [vs]Add MACsec Filters Nov 18, 2020

kcudnik reviewed Nov 18, 2020

View reviewed changes

kcudnik requested changes Nov 18, 2020

View reviewed changes

Pterosaur changed the title ~~[vs]Add MACsec Filters~~ [vslib]Add MACsec Filters Nov 19, 2020

Polish code

c21d2c9

Signed-off-by: Ze Gan <[email protected]>

Pterosaur force-pushed the macsec_filter branch from 362725b to c21d2c9 Compare November 19, 2020 04:03

Pterosaur mentioned this pull request Nov 19, 2020

[vslib]Add MACsec Manager #715

Merged

Pterosaur requested a review from kcudnik November 20, 2020 07:05

Pterosaur added 2 commits November 20, 2020 15:11

Rename member variable to follow the naming convention

d08c6a5

Signed-off-by: Ze Gan <[email protected]>

Change buffer length from ssize_t to size_t

854a490

Signed-off-by: Ze Gan <[email protected]>

kcudnik approved these changes Nov 20, 2020

View reviewed changes

kcudnik merged commit f1b1f05 into sonic-net:master Nov 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vslib]Add MACsec Filters #713

[vslib]Add MACsec Filters #713

Pterosaur commented Nov 18, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 20, 2020

Pterosaur Nov 20, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020

kcudnik Nov 18, 2020

Pterosaur Nov 19, 2020


		void enable_macsec_device(bool enable);

		void set_macsec_fd(int macsecfd);


		using namespace saivs;

		MACsecEgressFilter::MACsecEgressFilter(_In_ const std::string &macsecInterfaceName):

[vslib]Add MACsec Filters #713

[vslib]Add MACsec Filters #713

Conversation

Pterosaur commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment